feat(storage): durable merge gate requests for schema-mutating applies - #867

🤖 Review findings - created by Kiran's code review agent - for pull/867, 5bce323.

Verdict: 9 findings — 1 blocking (rebase-and-port onto main's sqlstore layout + full CI), 6 non-blocking, 2 suggestions.

Blocking

1. The PR targets a pre-refactor storage layout; it needs a rebase onto main with an internal/sqlstore port, a pkg/schema/postgres twin, and a green full CI run before merge. Since this PR's base, main (refactor(storage): depend on storage.Storage instead of *mysqlstore.Storage #947/feat(storage): add PostgreSQL EnsureSchema bootstrapper #946) moved the storage implementation into dialect-generic pkg/storage/internal/sqlstore — origin/main:pkg/storage/mysqlstore/mysqlstore.go is now a one-file shim (type Storage = sqlstore.Storage with var _ storage.Storage = (*Storage)(nil)) — and added 13 per-table pkg/schema/postgres/*.sql twins plus internal/sqlstore/parity_test.go cross-dialect coverage. This PR implements the store as package mysqlstore (merge_gate_requests.go#L3) with a mysql-only schema file, and widens the required interfaces — storage.go#L66 MergeGateRequests() MergeGateRequestStore, storage.go#L183 GetByTarget, storage.go#L192 MarkBlockedForFailedRefresh — with no sqlstore implementation. git merge-tree already predicts a content conflict in pkg/storage/internal/sqlstore/storage.go, and full CI never ran at this head (only DCO/Semgrep/zizmor completed; 3 of 32 checks). Failure scenario: resolved without a real port, internal/sqlstore.Storage no longer satisfies storage.Storage → repo-wide compile failure; a mechanical file-move compiles but leaves no pkg/schema/postgres/merge_gate_requests.sql and no parity coverage, so the durable merge-gate outbox silently has no PostgreSQL path (parity suite fails, or the table is absent on PG deployments and every Record() errors at runtime).

Non-blocking

1. MergeGateRequestStore omits the attempt-refunding Release its webhook template has, so shutdown-interrupted claims permanently consume retry budget. ClaimNext burns an attempt at claim time (merge_gate_requests.go#L149 SET state = ?, attempts = attempts + 1, ...), the claim predicate stops reclaiming at attempts < ? (merge_gate_requests.go#L90-L91, cap 5), and TerminateStuckProcessing then terminally fails the row — which the backfill sweep cannot rescue (it requires r.id IS NULL, merge_gate_requests.go#L284). The sibling WebhookEventStore.Release doc (storage.go#L283-L288) states the exact rationale: "an interrupted claim must not consume retry budget, or repeated deploy restarts could terminally fail a delivery". MergeGateRequestStore copied everything but that. Failure scenario: a long fan-out in flight during routine rolling deploys is claim-interrupted 5 times with zero real failures → the request terminally fails, the fan-out never runs, and sibling PRs keep stale green merge gates — the exact tier-0 failure this workstream exists to close. No consumer exists at this head, so adding Release alongside the feat(github): re-plan sibling PR checks when an apply changes a target schema #866 processor is a safe follow-up.

2. The contract and backfill sweep cover only fully-completed applies, so a failed multi-op apply that already executed some DDL mutates the live schema without ever generating a merge gate request — undocumented as a scope exclusion. The sweep filters WHERE a.state = ? bound to Completed (merge_gate_requests.go#L282), and a multi-op apply with one completed op and one failed op derives to failed (apply.go#L99 if counts[Apply.Failed] > 0 wins over #L117). Failure scenario: op1 ALTER TABLE lands, op2 fails → live schema changed, apply state failed, invisible to both feat(api): record durable merge gate requests at apply drive tails #868's completed-only recording and this sweep → siblings planned against the pre-apply schema keep stale green checks — contradicting the PR body's "no schema mutation ... can leave a sibling PR holding a stale verdict". May be an accepted stack scope decision, but nothing documents it; at minimum the contract at storage.go#L315 should state the exclusion.

3. MarkBlockedForFailedRefresh unconditionally overwrites blocking_reason, laundering away the review-time deployment-drift sentinel. checks.go#L541 sets blocking_reason = ? with only head-SHA and in-progress-apply guards (#L546-L547); a drift-blocked row matches and is relabeled. The sentinel is load-bearing twice: UpsertPlanResult's not-evaluated CASE guard keys every column on it (checks.go#L131), and the apply-start drift refusal compares the exact sentinel (webhook/apply_check_records.go#L79). Failure scenario: a drift-blocked row gets relabeled by a failed re-plan → subsequent not-evaluated writes can clear the block without any rollup re-run, and the apply-start refusal no longer fires. Fail-closed initially (row stays blocked), but the drift invariant — "only a write that re-ran the rollup rewrites those columns" — is silently broken. No caller exists until feat(github): re-plan sibling PR checks when an apply changes a target schema #866; the guard belongs in this primitive before feat(github): re-plan sibling PR checks when an apply changes a target schema #866 merges.

4. MarkBlockedForFailedRefresh lacks the newer-applies guard its siblings carry, so a same-head completed apply can be stomped and its apply_id marker erased. The only guards are AND head_sha = ? and AND NOT (status = ? AND apply_id IS NOT NULL) with checkStatusInProgress (checks.go#L546-L547), and #L537 does SET apply_id = NULL. Failure scenario: the feat(github): re-plan sibling PR checks when an apply changes a target schema #866 processor reads a row via GetByTarget; the PR's own apply starts and completes (CompleteForApply writes completed/success + apply_id at the same head); the processor's failed re-plan then stomps it to blocked and nulls apply_id. The mis-flip itself is fail-closed, but the nulled marker means MarkStalePlanSuccessful's apply_id IS NULL guard (checks.go#L249) can later convert an applied-then-removed change to success by cleanup alone. Siblings CompleteForApply/MarkActionRequiredForApply carry a newer.id > ? NOT EXISTS guard for exactly this TOCTOU class (checks.go#L366, #L435).

5. MarkBlockedForFailedRefresh infers "flipped" purely from RowsAffected with no 0-rows re-read, so under production changed-rows semantics an idempotent retry is indistinguishable from "row preserved". checks.go#L561 return rows > 0, nil. The package's own convention doc (mysql_test.go#L51-L54) warns that production connections report changed rows, and this same PR's mergeGateLeaseResult (merge_gate_requests.go#L322) plus the pre-existing MarkStalePlanSuccessful both re-read on 0 rows to disambiguate. Failure scenario: the processor flips a check, commits, crashes before posting the GitHub check-run update; the retry writes identical values → RowsAffected 0 → (false, nil); a caller treating false as "skip the GitHub update" leaves GitHub showing the stale green verdict while the DB says blocked. TestCheckStore_MarkBlockedForFailedRefresh runs only on the clientFoundRows testDB so cannot catch this.

6. The merge gate lease tests never run under production changed-rows semantics, leaving the mergeGateLeaseResult owned-but-unchanged fallback untested. The fallback branch if currentToken.Valid && currentToken.String == leaseToken { return nil } (merge_gate_requests.go#L338) is reachable only under changed-rows semantics; the "same-token retry is a no-op" assertion (merge_gate_requests_test.go#L217) passes trivially on the clientFoundRows connection without exercising it. The webhook analog pins SET timestamp and opens testDSNChangedRows for exactly these paths (TestWebhookEventStore_TerminalWritesAreIdempotentOnRetry, TestWebhookEventStore_HeartbeatTreatsUnchangedMatchingLeaseAsSuccess); grep shows zero testDSNChangedRows uses in merge_gate_requests_test.go. Correct today, but a future edit to mergeGateLeaseResult could regress the production path silently.

General suggestions

• PendingForTarget/CompletePendingCoalesced rely on caller ordering discipline encoded only in a comment (storage.go#L333-L337): the query has no created-at/id cutoff (merge_gate_requests.go#L180), so a processor that lists after its re-plan snapshot could coalesce-complete a request recorded mid-fan-out whose schema change never triggered a refresh. A cutoff parameter captured at claim time (max id / claim timestamp) would enforce the invariant at the storage layer instead of by feat(github): re-plan sibling PR checks when an apply changes a target schema #866-caller discipline.
• Field mergeGatees breaks the field-matches-store-name convention — storage.go#L27 mergeGatees *mergeGateRequestStore (also lines 46, 112); every sibling field is named after its store (controlRequests, webhookEvents, checks). mergeGateRequests would match, and the irregular name will be missed by grep sweeps for the ongoing merge-gate rename that each stacked PR carries.

The one thing that could have broken, verified

The lease state machine in merge_gate_requests.go — the interaction of mergeGateClaimablePredicate (pending OR failed+retry_after-elapsed+under-cap OR processing+lease-expired+under-cap, #L87-L103), ClaimNext's two-step FOR UPDATE SKIP LOCKED claim with token rotation (#L105-L174), and mergeGateLeaseResult's 0-rows token-recheck fallback (#L322). A flaw here silently drops or double-runs merge gate fan-outs, i.e. leaves stale green tier-0 checks — the exact failure the workstream exists to close. Proved sound three ways: (a) line-by-line diff against the battle-tested webhook_events.go analog — the only deltas are intentional (no started_at, no Release (non-blocking #1), a single failed state keyed on retry_after NULL-ness whose terminal branch is provably unclaimable); (b) direct verification of the concurrency invariants — the claim tx holds the row lock through commit so the post-commit struct reflection cannot drift, reclaim always rotates lease_token so a stale owner's write affects 0 rows and the token re-read returns ErrMergeGateLeaseLost, and TerminateStuckProcessing NULLs the token so a zombie driver's late heartbeat also loses; (c) executing the full new integration suite against a real MySQL 8.0 testcontainer at this exact head (ok github.com/block/schemabot/pkg/storage/mysqlstore 9.7s) — which matters because CI never ran it. The residual risk is not the state machine but the landing surface: none of this code exists in main's new internal/sqlstore layer (the blocking finding); the remaining proof obligation is the rebase-and-port plus a green full 32-check run.

Verified correct

• CI status fact: only 3 of 32 checks (DCO, Semgrep, zizmor) completed at head 5bce323 — the full suite never executed, so every no-behavior-change claim below was re-verified locally rather than taken from CI. The PR is also currently CONFLICTING with main (git merge-tree predicts content conflicts in pkg/storage/internal/sqlstore/storage.go and pkg/webhook/durable_check_run_test.go).
• All 11 new integration tests (9 TestMergeGateStore_* plus TestCheckStore_GetByTargetSpansRepositories and TestCheckStore_MarkBlockedForFailedRefresh) pass locally against a real MySQL 8.0 testcontainer at this head.
• Full toolchain verification at this head: go build ./..., go vet ./..., go vet -tags=integration ./..., go vet -tags=e2e ./... all pass — the Storage interface widening at storage.go#L66 reached every implementer, including the api mockStorage and all webhook test fakes via interface embedding.
• The WebhookProviderGitHub → ProviderGitHub rename is a pure compile-time constant rename with identical value "github"; grep over the whole worktree returns zero stale references across the 40+ call sites.
• The claimable predicate is internally consistent with the single-failed-state design: 5 placeholders bound in exact arg order with correct timestamp precisions; terminal failure (retry_after NULL) is never claimable; both reclaim branches are gated on attempts < MaxMergeGateAttempts; each branch has a dedicated integration test.
• TerminateStuckProcessing (merge_gate_requests.go#L303) is a field-for-field mirror of the webhook version, including lease/retry_after clearing, COALESCE'd completed_at, and token NULLing so a zombie's late heartbeat gets lease-lost.
• SQL/scan consistency: mergeGateColumns order matches Record's INSERT list and scanMergeGateRequestInto's Scan order; nullable columns use sql.Null*; lease_expires_at datetime(6) vs NOW(6) and retry_after datetime vs second-precision NOW() match webhook conventions; table ENGINE/CHARSET/COLLATE matches all 13 sibling schema files.
• Schema plumbing is automatic: both the test harness (applyTestSchema) and production readEmbeddedSchemaFiles glob the embedded pkg/schema/mysql dir, and clearTables iterates SHOW TABLES — merge_gate_requests.sql needs no registration and cannot leak state between tests.
• Indexes cover every new query with no redundancy: idx_merge_gate_apply UNIQUE(apply_id) backs Record's duplicate-key idempotency, GetByApplyID, and the sweep's join; idx_merge_gate_claimable backs ClaimNext; idx_merge_gate_target backs PendingForTarget; the new checks idx_env_db backs the repo-less GetByTarget and is not a left-prefix of any existing composite.
• MarkBlockedForFailedRefresh's mechanics are sound per its stated contract: 13 placeholders = 13 args in order; the flip direction is fail-closed (blocking), never converting uncertainty into a pass; all three predicate branches are covered by its test (the gaps above are guards/semantics beyond that contract).
• GetByTarget's repo-less semantics (CLI/gRPC applies carry no repository) are tested cross-repository in TestCheckStore_GetByTargetSpansRepositories.
• FindCompletedAppliesMissingRequest's LEFT JOIN ... r.id IS NULL cannot duplicate applies (apply_id UNIQUE), correctly excludes NULL completed_at via the interval comparison, and reuses the shared apply scan helpers; each exclusion is covered by its test.
• The rewritten integration/operator_test.go assertion preserves the parent-failed invariant via polling with state.IsState (the AGENTS.md-mandated comparison) and correctly loosens an assertion that raced the operator's separate parent-state re-derivation write.
• Stacked-consumer fit: feat(github): re-plan sibling PR checks when an apply changes a target schema #866's processor calls exactly and only the methods this PR defines, and feat(api): record durable merge gate requests at apply drive tails #868's drive tail uses Record/GetByApplyID with the documented recorded=false idempotency contract; no stack-order inversion — this head builds standalone.
• The CI timeout bump (test.yaml 10 → 15 min) matches the genuinely added container-backed test work (a 475-line MySQL-testcontainer suite), not flake-masking; no in-test poll deadlines were increased.

This review was generated by Claude Code (claude-fable-5).